Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

Claude finished @behnam-deriv's task —— View job

Code Review Complete

Tasks

• Read the changed file and understand the revert
• Review git diff to see exact changes
• Investigate MarkerArea and MarkerSeries to understand rebuild behavior
• Analyze potential regressions and edge cases
• Provide comprehensive review feedback

Summary

This PR reverts the fix from #476 which added ObjectKey-based remounting for marker-related widgets. While the revert simplifies the code, it likely reintroduces the original bug that #476 was designed to fix.

Critical Issues (High Priority)

1. Potential Regression: MarkerArea State Persistence Bug

Location: lib/src/deriv_chart/chart/main_chart.dart:582-595

Issue: The original PR #476 added explicit keying to force widget remounting when markerSeries changes. The commit message stated:

"LayoutBuilder only re-invokes its builder on constraint changes; without a key that tracks markerSeries identity, the old MarkerArea subtree persists after trade-type switches."

By removing both:

1. ObjectKey(widget.markerSeries) from the LayoutBuilder (line 401)
2. KeyedSubtree wrapper with ObjectKey(widget.markerSeries) from _buildMarkerArea() (line 582)

The widget tree may not properly rebuild when switching between different marker series instances, particularly during trade-type changes.

Root Cause Analysis:

Looking at marker_area.dart:41-57, the _MarkerAreaState has stateful behavior:

• gestureManager is registered in initState() with a callback: gestureManager.registerCallback(_onTap)
• The callback is removed in dispose(): gestureManager.removeCallback(_onTap)

When markerSeries changes identity but the widget doesn't remount:

• The old _MarkerAreaState instance persists
• The old gesture callbacks remain registered
• The widget continues to reference the old markerSeries in its closure context
• This can lead to stale tap handlers and incorrect marker interactions

Evidence from code:

// marker_area.dart:59-69
void _onTap(TapUpDetails details) {
  final MarkerSeries series = widget.markerSeries; // <-- References current widget's series
  
  if (series.activeMarker != null) {
    if (series.activeMarker!.tapArea.contains(details.localPosition)) {
      series.activeMarker!.onTap?.call();
    } else {
      series.activeMarker!.onTapOutside?.call();
    }
    return;
  }
  // ...
}

While the build() method calls widget.markerSeries.update() on each rebuild, the gesture callback registration happens only once in initState(), which means the callback closure may capture stale references.

Impact:

• Medium-High severity: Trade type switches may result in incorrect marker behavior
• Users tapping markers after a trade type change might trigger callbacks for the wrong series
• Visual inconsistencies between displayed markers and interactive tap areas

Recommendation:

• ❌ Do not merge this revert without first understanding why the original issue occurred
• 🔍 Investigate the root cause: Why does markerSeries change identity during trade-type switches?
• 🎯 Consider a better fix:
  • Override didUpdateWidget() in _MarkerAreaState to re-register callbacks when markerSeries changes
  • Use a more stable identity for markerSeries so it doesn't change on every rebuild
  • Add proper equality checks to avoid unnecessary remounting

Medium Priority Issues

2. Missing Test Coverage

Issue: There are no automated tests for marker rebuild behavior. This makes it:

• Difficult to verify the original bug exists
• Impossible to prevent regressions
• Hard to validate alternative fixes

Recommendation:

• Add widget tests that verify correct behavior when markerSeries changes
• Test gesture callback behavior after marker series updates
• Consider integration tests for trade-type switching scenarios

3. Unclear Justification for Revert

Issue: The PR description only states "Reverts #476" without explaining:

• Why the original fix was problematic
• What alternative solution is planned
• Whether the original issue has been verified as non-existent

Recommendation:

• Document in the PR description why the revert is necessary
• Explain what problems the original fix caused (performance, bugs, architecture concerns)
• Outline a plan for addressing the original issue differently

Low Priority Issues

4. Code Comments Removed

Location: lib/src/deriv_chart/chart/main_chart.dart:401

The original fix included helpful documentation:

// Force remount when markerSeries changes.
// LayoutBuilder only re-invokes its builder on constraint changes;
// without a key that tracks markerSeries identity, the old
// MarkerArea subtree persists after trade-type switches.

Impact: Future developers lose context about this subtle Flutter framework behavior.

Recommendation: If the revert is necessary, consider adding a comment explaining why keying is NOT needed in this case.

Alternative Solutions to Consider

Instead of reverting, consider these approaches:

1. Lifecycle-based callback management:

@override
void didUpdateWidget(MarkerArea oldWidget) {
  super.didUpdateWidget(oldWidget);
  if (oldWidget.markerSeries != widget.markerSeries) {
    gestureManager
      ..removeCallback(_onTap)
      ..registerCallback(_onTap);
  }
}

2. Stable series identity: Ensure markerSeries maintains the same instance identity unless truly changing, using proper equality checks upstream.

3. ValueKey instead of ObjectKey: If only certain properties matter, use ValueKey(widget.markerSeries.id) for more predictable behavior.

Testing Recommendations

Before merging, please verify:

1. ✅ Trade type switching works correctly and markers respond to taps properly
2. ✅ Marker tap callbacks invoke the correct handlers after trade type changes
3. ✅ Active marker highlighting works correctly after series updates
4. ✅ No memory leaks from unreleased gesture callbacks

Conclusion

RECOMMENDATION: Do not merge without further investigation.

While code simplification is valuable, this revert appears to reintroduce a real bug related to stateful widget lifecycle management. The original fix may have been crude (forcing remounting), but it addressed a legitimate issue with gesture callback staleness.

I recommend:

1. Reproduce the original bug to confirm it exists
2. Implement a more surgical fix using didUpdateWidget()
3. Add test coverage to prevent future regressions
4. Document the reasoning in code comments

Reverting this since it breaks the drawing tools functionality on web